[https://nvbugs/6484986][test] add UCX cancellation regression coverage#16688
[https://nvbugs/6484986][test] add UCX cancellation regression coverage#16688chienchunhung wants to merge 4 commits into
Conversation
|
/bot run --disable-fail-fast --stage-list "A100X-PyTorch-1" |
|
PR_Github #60792 [ run ] triggered by Bot. Commit: |
|
PR_Github #60792 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "A100X-PyTorch-1" |
|
PR_Github #60833 [ run ] triggered by Bot. Commit: |
|
PR_Github #60833 [ run ] completed with state |
|
/bot run --disable-fail-fast |
5cba0c2 to
2fd0b71
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61024 [ run ] triggered by Bot. Commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesUCX receive cancellation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UcxCommTest
participant UcxConnectionManager
participant TransferContext
participant UCXRequest
UcxCommTest->>UcxConnectionManager: start recvConnect asynchronously
UcxConnectionManager->>TransferContext: check termination flag
UcxConnectionManager->>UCXRequest: poll pending receive
UcxCommTest->>TransferContext: set termination flag
UcxConnectionManager->>UCXRequest: cancel receive
UCXRequest-->>UcxConnectionManager: return completion status
UcxConnectionManager-->>UcxCommTest: return nullptr
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/tests/unit_tests/executor/ucxCommTest.cpp (1)
101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply the C++ const and brace-style rules.
Declare the unmodified
connectionManageranderrorvariablesconst, and put the lambda opening brace on its own line.Proposed fix
- auto connectionManager = makeOneUcxConnectionManager(); + auto const connectionManager = makeOneUcxConnectionManager(); ... - [&]() { + [&]() + { ... - std::string error = e.what(); + std::string const error = e.what();As per coding guidelines, “declare unmodified variables const” and “use Allman braces.”
Also applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/unit_tests/executor/ucxCommTest.cpp` around lines 101 - 107, Update the test setup around makeOneUcxConnectionManager and the related error declaration to mark unmodified variables const. Reformat the std::async lambda associated with receiveFuture, and the additionally referenced lambda, so each opening brace appears on its own line, following the project’s Allman brace style.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/unit_tests/executor/ucxCommTest.cpp`:
- Around line 120-129: Update the exception rethrow in the catch block to use a
bare throw statement instead of throwing the caught variable, preserving the
original exception type and diagnostics while leaving the UCX skip logic
unchanged.
- Around line 97-118: Update recvConnectCancellation to guard the asynchronous
recvConnect probe with an external per-test or process-level timeout, ensuring a
cancellation failure terminates or reports promptly without allowing the
std::async future destructor to block indefinitely after the timeout assertion.
---
Nitpick comments:
In `@cpp/tests/unit_tests/executor/ucxCommTest.cpp`:
- Around line 101-107: Update the test setup around makeOneUcxConnectionManager
and the related error declaration to mark unmodified variables const. Reformat
the std::async lambda associated with receiveFuture, and the additionally
referenced lambda, so each opening brace appears on its own line, following the
project’s Allman brace style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43d0fd11-d342-4cc5-a0f5-af0321ce757b
📒 Files selected for processing (3)
cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cppcpp/tests/unit_tests/executor/ucxCommTest.cpptests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
PR_Github #61024 [ run ] completed with state
|
2fd0b71 to
87561d8
Compare
|
/bot run --disable-fail-fast |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/unit_tests/executor/ucxCommTest.cpp`:
- Line 101: Declare the unmodified local variables in the affected test cases as
const: change connectionManager in makeOneUcxConnectionManager usage to auto
const and declare error as std::string const. Apply this consistently at both
referenced locations without changing their behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb26e44c-3d68-47ea-a52e-6b10818f2bac
📒 Files selected for processing (3)
cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cppcpp/tests/unit_tests/executor/ucxCommTest.cpptests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cpp
|
/bot run --disable-fail-fast |
|
PR_Github #61204 [ run ] triggered by Bot. Commit: |
|
PR_Github #61169 [ run ] completed with state |
|
PR_Github #61204 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61315 [ run ] triggered by Bot. Commit: |
|
PR_Github #61315 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61383 [ run ] triggered by Bot. Commit: |
|
PR_Github #61383 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
a039a8d to
ab8f928
Compare
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
ab8f928 to
0fded85
Compare
chienchunhung
changed the title
|
/bot run --disable-fail-fast --stage-list "A100X-PyTorch-1" |
chienchunhung
changed the title
|
PR_Github #61615 [ run ] triggered by Bot. Commit: |
|
PR_Github #61615 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #61640 [ run ] triggered by Bot. Commit: |
|
PR_Github #61640 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61665 [ run ] triggered by Bot. Commit: |
|
PR_Github #61665 [ run ] completed with state
|
5 participants
Summary
recvConnect()during shutdown.Context
The production fix for the UCX shutdown deadlock is now present on
mainthrough PR #13055. ItsrecvConnect()implementation keeps the receive state alive through the UCXX callback, observes the existing transfer-termination flag, cancels the outstanding receive, and returns without waiting indefinitely for asynchronous cancellation completion.After rebasing onto that implementation, this PR no longer replaces the production cancellation logic. It retains the regression coverage and waiver cleanup needed to verify the reported failures remain fixed.
Changes
UcxCommTest.recvConnectCancellation, which starts a pending receive, requests termination, and verifies thatrecvConnect()returnsnullptrwithin five seconds.add_gtest()with an optionalTIMEOUTproperty and setucxCommTestto 60 seconds, ensuring a cancellation regression fails cleanly.unittest/llmapi/test_llm_pytorch.py::test_llm_context_only_timed_out[None](NVBUG#6484986).unittest/llmapi/test_llm_pytorch.py::test_llm_context_only_timed_out_kv_cache_exhausted[None-UCX-100](NVBUG#6488811).[None-UCX-1000]waiver associated with NVBUG#6490004.Verification
Current rebased head:
0fded855fonmaincommit121cf56ae.python3 scripts/check_test_list.py --check-duplicate-waives --validate: PASSED with 2,035 unique test entries and no duplicate waivers.git diff --check upstream/main...HEAD: PASSED.A100X-PyTorch-1pipeline #49822 completed successfully on commit0fded855fwith 132 passed, 0 failed, and 15 skipped tests.unittest/llmapi/test_llm_pytorch.py::test_llm_context_only_timed_out[None]: PASSEDunittest/llmapi/test_llm_pytorch.py::test_llm_context_only_timed_out_kv_cache_exhausted[None-UCX-100]: PASSEDA100X-PyTorch-1verification on pre-rebase commit5cba0c29b: pipeline #49109 completed with 133 passed, 0 failed, and 14 skipped tests; both affected cases also passed.The two exact post-rebase per-test records in build 49822 report
PASSEDon A100X. They executed in the targeted stage rather than being skipped or waived, so the NVBUG#6484986 and NVBUG#6488811 waivers can remain removed.